fix(runtime): drain steerQueue at top of RunStream loop to close idle-window race#2492
Conversation
…-window race The steer queue was only drained mid-loop (after tool-call batches). Two gaps allowed steer messages to be silently dropped: 1. Idle-window race: a Steer call lands between two RunStream invocations; the queue holds the message but nothing drains it when the next RunStream starts. 2. First-turn miss: when a RunStream turn produces a plain-text response with no tool calls, res.Stopped fires before the mid-loop drain is reached; the pending steer message is never injected. Fix: drain steerQueue at the top of the for-loop body (before the first model call) using the same pattern as the existing mid-loop drain. Also extract the duplicate system-reminder wrap string into wrapSteerMessage so the two drain sites cannot drift apart. Also handles the empty-session bootstrap case: a RunStream started with zero messages but steer messages already queued injects those messages before the first model call, giving the model its initial context. Tests added: - TestSteer_IdleWindowIsConsumedOnNextTurn - TestSteer_EmptySessionBootstrap Fixes docker#2491 Assisted-By: docker-agent
| // a plain-text response with no tool calls. | ||
| if steered := r.steerQueue.Drain(ctx); len(steered) > 0 { | ||
| for _, sm := range steered { | ||
| userMsg := session.UserMessage(wrapSteerMessage(sm.Content), sm.MultiContent...) |
There was a problem hiding this comment.
start of turn message should not get the steerMessage envelope (the agent is not working on anything at this point)
The system-reminder envelope is only appropriate when the agent is actively working (mid-loop, between tool calls). At the top of a new turn the runtime is idle, so steer messages are injected as plain user messages — no wrapping needed. Assisted-By: docker-agent
|
Fixed — top-of-turn steer messages are now injected as plain user messages (no system-reminder envelope). The wrapper is kept only on the mid-loop drain where the agent is actively working. |
|
Updated: top-of-turn steer messages are now injected as plain user messages without the system-reminder envelope. The |
- Move top-of-turn drain to after AgentInfo and contextLimit init so compactIfNeeded can be called immediately after injection (S1, S2). - Extract appendSteerAndEmit helper (wrap bool) to eliminate the duplicated add-message+emit-event idiom at both drain sites (S3). - Tighten both steer tests to assert the stored session message and the message seen by the model contain raw content with NO system-reminder envelope for the top-of-turn path (B1). - Document messageRecordingProvider fallback behaviour for unexpected extra model calls (N4). Assisted-By: docker-agent
|
Addressed all review findings:
|
A Steer() call arriving in the narrow window between the mid-loop drain and the res.Stopped break was stranded until the next RunStream call. Fix: re-check steerQueue inside the res.Stopped block before breaking. Any message that enqueued successfully is now guaranteed to be consumed within the current RunStream. Messages injected here use the system-reminder envelope (matching mid-turn semantics: the agent just finished a turn). T1: both existing top-of-turn steer tests already assert NotContains(<system-reminder>) in firstCallMsgs — confirmed present. Test added: TestSteer_EndOfIterationRaceIsConsumedInCurrentRunStream Uses a provider hook to inject a Steer() synchronously during CreateChatCompletionStream on the first call, simulating the race. Asserts: steer UserMessageEvent emitted within the same RunStream, exactly 2 model calls made, and stored session message carries the system-reminder envelope. Assisted-By: docker-agent
| // same system-reminder envelope as mid-turn steers. | ||
| if steered := r.steerQueue.Drain(ctx); len(steered) > 0 { | ||
| for _, sm := range steered { | ||
| r.appendSteerAndEmit(sess, sm, true, events) |
There was a problem hiding this comment.
we are about to stop, those messages should not have an enveloppe
… test At res.Stopped the current turn is ending and a new one is about to begin — the agent is not mid-task — so steer messages injected there should be plain user messages, same as the top-of-turn drain. Changes: - loop.go: appendSteerAndEmit(wrap=false) at the end-of-iteration drain (inside res.Stopped block); update comment accordingly. - runtime_test.go: add hookStream type that fires a callback on the FinishReasonStop Recv, giving a deterministic injection point; rewrite TestSteer_EndOfIterationRaceIsConsumedInCurrentRunStream to use hookStream and assert consumed-in-same-RunStream invariant (2 model calls, UserMessageEvent present, steer content in session). Assisted-By: docker-agent
|
Addressed inline change request (discussion_r3123189465): end-of-iteration drain now uses |
….Contains - hookStream: add blank line separating embedded *mockStream from onStop field (embeddedstructfieldcheck) - TestSteer_EndOfIterationRaceIsConsumedInCurrentRunStream: replace assert.True(strings.Contains(...)) with assert.Contains (testifylint) Assisted-By: docker-agent
…envelope caveat Add assert.Contains on the stored session message content in TestSteer_EndOfIterationRaceIsConsumedInCurrentRunStream, bringing it closer to parity with the other two steer tests. The Equal/NotContains envelope assertions cannot be applied here: the hookStream injection fires inside Recv() on the stop chunk, so the steer lands in the queue before tryModelWithFallback returns and is always consumed by the mid-loop drain (wrap=true). The end-of- iteration drain (wrap=false, inside res.Stopped) covers the narrower race reachable only via real goroutine concurrency. A comment in the test documents this constraint. Assisted-By: docker-agent
|
After discussion, we think we should remove all wrapping: depending on the user message it can create too much confusion. Remove the message wrapper alltogether |
All three steer drain sites now inject plain user messages. Remove wrapSteerMessage helper and the wrap bool parameter from appendSteerAndEmit — there is no wrapping anywhere. Update code comments and PR body to reflect the simplified semantics. Tighten TestSteer_EndOfIterationRaceIsConsumedInCurrentRunStream to use assert.Equal + assert.NotContains now that the mid-loop drain no longer wraps, removing the stale wrap=true/wrap=false caveat comment. Assisted-By: docker-agent
simonferquel
left a comment
There was a problem hiding this comment.
Be more concise in your code comments
Cut multi-line explanations to one short line each at all three drain sites and on appendSteerAndEmit. Assisted-By: docker-agent
simonferquel
left a comment
There was a problem hiding this comment.
lgtm, asking for a 2nd review
|
|
||
| // Drain steer messages queued while idle or before the first model call | ||
| // (covers idle-window and first-turn-miss races). | ||
| if steered := r.steerQueue.Drain(ctx); len(steered) > 0 { |
There was a problem hiding this comment.
I don't understand why this one is needed here?
There was a problem hiding this comment.
2 cases:
- if a user sends a steering message while the turn is being ended, you end up with the steering messages being sent after the next tool call of the next turn instead of immediately
- when used as a library, in an app that consider all messages as "interrupting" - a la Claude Code - this simplify things a lot: you do not have to treat messages sent while the runner is idle differently than when it is active: they all come from the same queue
There was a problem hiding this comment.
I'm not talking about the case when the turn is being ended, that's lower in the code. I'm asking why are we steering before the turn starts
|
Re: #2492 (comment) The mid-loop drain and the A |
Ok but there is already a streer.drain that was added in the res.stopped case |
|
Exactly — that drain is the fix. The explanation was describing why it was needed, not a gap that still exists. The code is complete. Happy to get your re-approval if you have no further concerns. |
Fixes #2491
Problem
LocalRuntime.Steerenqueues intosteerQueue, but the queue was only drained mid-loop — after a batch of tool calls, before the stop check. Two gaps allowed steer messages to be silently dropped:Steercall lands between twoRunStreaminvocations. The queue holds the message but nothing drains it when the nextRunStreamstarts.RunStreamturn produces a plain-text response with no tool calls,res.Stoppedfires before the mid-loop drain is reached; the pending steer is never injected.Fix
Three steer drain points are added across the
RunStreamloop, giving the invariant: anySteercall that enqueues successfully is guaranteed to be consumed within the currentRunStream, never stranded.All three drain sites inject steer messages as plain user messages — no wrapping, no envelope.
AgentInfoandcontextLimitinit, beforeGetMessages): catches idle-window and first-turn-miss races.res.Stopped, before the follow-up queue andbreak): closes the narrow race between the mid-loop drain and the stop decision.The
appendSteerAndEmithelper centralises the add-message + emit-event idiom used at all three sites.Empty-session bootstrap
A
RunStreamstarted with zero messages in the session but steer messages already queued injects those messages as plain user messages before the first model call. The model receives them as its initial context.Changes
pkg/runtime/loop.go—appendSteerAndEmithelper (no wrapping); three drain sites.pkg/runtime/runtime_test.go—messageRecordingProvider,hookStream,steerInjectProvider; three focused tests:TestSteer_IdleWindowIsConsumedOnNextTurnTestSteer_EmptySessionBootstrapTestSteer_EndOfIterationRaceIsConsumedInCurrentRunStream